OCPEDGE-2118: Record Kubernetes events for etcd transition lifecycle#1653
OCPEDGE-2118: Record Kubernetes events for etcd transition lifecycle#1653lucaconsalvi wants to merge 5 commits into
Conversation
Add event recording to the TNF setup runner and etcd transition code so the full CEO-to-pacemaker handoff is observable via `kubectl get events -n openshift-etcd`. Nine events are emitted across the transition lifecycle: - runner.go: auth, cluster, fencing, etcd resource, constraints - etcd.go: transition started, waiting, static pod removed, completed Events use the direct K8s API (not library-go Recorder) since the setup runner is a short-lived Job. Failures are logged but never block the transition. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Skipping CI for Draft Pull Request. |
|
@lucaconsalvi: This pull request references OCPEDGE-2118 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughTNF setup now records Kubernetes events for configuration milestones and the etcd transition lifecycle. ChangesTNF transition events
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RunTnfSetup
participant RecordSetupEvent
participant RemoveStaticContainer
participant KubernetesEvents
RunTnfSetup->>RecordSetupEvent: record setup milestone
RunTnfSetup->>RemoveStaticContainer: hand over with kubeClient
RemoveStaticContainer->>RecordSetupEvent: record transition milestone
RecordSetupEvent->>KubernetesEvents: create Normal Event
Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/tnf/pkg/tools/events.go`:
- Around line 3-28: Update RecordSetupEvent so the reason component used in the
event metadata.name is converted to lowercase before constructing the name.
Preserve the existing event naming format and reason value used elsewhere in the
event.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 11fb1836-7157-46ed-9989-87c3820a66e7
📒 Files selected for processing (5)
hack/test-transition-events.shpkg/tnf/pkg/etcd/etcd.gopkg/tnf/pkg/etcd/etcd_test.gopkg/tnf/pkg/tools/events.gopkg/tnf/setup/runner.go
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
The event reason (e.g. EtcdTransitionAuthCompleted) was used directly in metadata.name which should be a valid lowercase DNS subdomain. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
fonta-rh
left a comment
fonta-rh
left a comment
There was a problem hiding this comment.
Ahhh, I'm amazed, I have nothing to nitpick. This does what it should minimally, good test coverage! Good work!
|
/lgtm |
|
Scheduling required tests: |
E2E Verification on Live TNF ClusterCluster: Deployed via cluster-bot ( Method: Verified all 9 transition events are emitted during the CEO-to-Pacemaker etcd handoff using ResultsAll 9 events recorded successfully in
/verified by @lucaconsalvi |
openshift-ci-robot
added
the
verified
|
@lucaconsalvi: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/retest |
1 similar comment
|
/retest |
|
/approve |
|
/retest |
1 similar comment
|
/retest |
TNF-specific manual validation script belongs under the TNF directory tree rather than the repo-wide hack/ directory. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci-robot
removed
the
verified
|
New changes are detected. LGTM label has been removed. |
|
@lucaconsalvi: This pull request references OCPEDGE-2118 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fonta-rh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/tnf/hack/test-transition-events.sh (1)
60-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReturn a non-zero exit code on verification failure.
If
grepfails to find anyEtcdTransitionevents, the script currently prints an error message but still exits with a0(success) code due to the|| echo ...fallback. For a validation script, it is a best practice to exit with a non-zero code to accurately reflect the failure.Additionally, the empty quotes in
--field-selector reason!==""are consumed by Bash and passed toocasreason!=. It is better to quote the entire argument or omit the empty quotes for clarity.♻️ Proposed refactor
-oc get events -n "${NAMESPACE}" --field-selector reason!=="" --sort-by='.lastTimestamp' | grep -i "EtcdTransition" || echo "No EtcdTransition events found!" +oc get events -n "${NAMESPACE}" --field-selector reason!= --sort-by='.lastTimestamp' | grep -i "EtcdTransition" || { echo "No EtcdTransition events found!"; exit 1; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/tnf/hack/test-transition-events.sh` at line 60, Update the EtcdTransition verification command to pass the field selector as a single correctly quoted argument, avoiding Bash stripping the empty quotes. Replace the grep fallback in the validation flow so missing EtcdTransition events exits with a non-zero status after reporting the failure, while preserving successful behavior when matching events are found.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/tnf/hack/test-transition-events.sh`:
- Line 63: Label the mock Event manifest metadata with tnf-test="true", then
update the cleanup instruction to use -l 'tnf-test=true' so only mock events are
targeted; apply both changes in pkg/tnf/hack/test-transition-events.sh at lines
33-52 and 63.
---
Nitpick comments:
In `@pkg/tnf/hack/test-transition-events.sh`:
- Line 60: Update the EtcdTransition verification command to pass the field
selector as a single correctly quoted argument, avoiding Bash stripping the
empty quotes. Replace the grep fallback in the validation flow so missing
EtcdTransition events exits with a non-zero status after reporting the failure,
while preserving successful behavior when matching events are found.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4adac7d9-d4a5-49d9-849c-cd337ac96d6b
📒 Files selected for processing (1)
pkg/tnf/hack/test-transition-events.sh
Add tnf-test=true label to mock events so the cleanup command only targets test events, not real cluster diagnostic data. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/retest |
|
/test e2e-aws-ovn-single-node |
|
/verified by @lucaconsalvi |
openshift-ci-robot
added
the
verified
|
@lucaconsalvi: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@lucaconsalvi: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/retest |
3 participants
Add event recording to the TNF setup runner and etcd transition code so the full CEO-to-pacemaker handoff is observable via
kubectl get events -n openshift-etcd.Nine events are emitted across the transition lifecycle:
Events use the direct K8s API (not library-go Recorder) since the setup runner is a short-lived Job. Failures are logged but never block the transition.
Summary by CodeRabbit